Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent million-proposal attack by introducing minimum stake req… #2805

Merged
merged 10 commits into from
Jun 15, 2020

Conversation

bowenwang1996
Copy link
Collaborator

…uirements

Fixes #2610 in two parts:

  1. Introduce a minimum_stake_divisor to have a lower limit for staking transactions. If someone tries to stake with less than last_seat_price / minimum_stake_divisor, the transaction will fail.

  2. Instead of copying the proposals in every block info, use an aggregator that always updates the proposal information to the last final block and at the end of an epoch, process the rest between last final block and last block of the epoch.

Test plan

  • Unit tests in epoch manager to make sure that the change in how we store aggregated information doesn't break anything.
  • Tests in neard/src/runtime to make sure that the proposals that are now invalid are properly rejected.

@gitpod-io
Copy link

gitpod-io bot commented Jun 6, 2020

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not that familiar with Epoch Manager, so I just skimmed over the code. Overall, looks fine to me.

/// The minimum stake required for staking is last seat price divided by this number.
#[serde(default = "default_minimum_stake_divisor")]
#[default(10)]
pub minimum_stake_divisor: u64,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using Rational instead of encoding the 1/divisor implicitly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's needed (at least for now) because the stake is usually a large number (due to yotcoNEAR) and for example there would very little difference if we divide by 33 or multiple by 3/100.

@bowenwang1996 bowenwang1996 requested a review from mikhailOK June 9, 2020 15:03
Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for my side

chain/epoch_manager/src/lib.rs Outdated Show resolved Hide resolved
chain/epoch_manager/src/lib.rs Show resolved Hide resolved
chain/epoch_manager/src/lib.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Excessive copying of ValidatorStake in epoch manager
4 participants